-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update modal animation #64580
Update modal animation #64580
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @nick-a8c. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +24 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the blur filter was intentionally disabled in #43929 because it caused performance issues.
This issue seems to be discussed in #43877. I don't have time to read into this issue in depth today, but if we were to reintroduce blur, we would need to make sure that there were no performance issues.
Looking so much better! The background blur is a nice touch. I was wondering how it might look it the travel distance was even smaller - like 90% scale. The travel distance should be from 90% scale to 100%. So I wonder will it make a subtle difference changing it from 80% to 90%. |
Flaky tests detected in 30773e4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10614980603
|
I tested this PR.
The situation may have changed since the lag issue was reported, but I would be grateful if you could test it to see if the lag still occurs. b05daf32751c13684b13e69553c32ee9.mp4 |
Visually, I like the blur. UX-wise I don't. Sometimes it is handy to still see the names of the already created pages behind the modal. |
Please o please do not blur the background as it is as @hanneslsm says can be useful on occasion to still be able to see the background while the modal is open. A blurred background creates disconnection. If one steps away for moment while the modal is open and then returns to the page one might be wondering.... what was I doing here. The modal still needs to keep connection with the rest of the page. So in general for modals please keep the connection with the background as it creates a safety/ a hint for the user seeing the connection of modal and the rest of the page. |
I agree that it is useful for users who can see the screen to be able to see the content behind an open modal. However, a modal is essentially a dialog element and should be a document independent of its background. Users who rely solely on screen readers and keyboard navigation cannot access the content outside the modal. In other words, I think the modal itself needs to convey sufficient intent. On the other hand, blurring the background has the added benefit of allowing users to focus on the content inside the modal. If it is difficult to understand the intent of the modal without the background content, it might be that the modal itself is not providing a sufficient content, and the modal content might need to be reconsidered. However, I am not an accessibility expert, so I would like to hear other people's opinions. |
I agree with @t-hamano — technically, the Having said that, I understand if folks still prefer to avoid blurring the background. |
Appreciate sweating the details. In this case, although the animation curve feels slightly nicer (at a glance), removing the directionality by replacing the translate with a scale doesn't seem like a win. I always think of animation as orchestration according to a physical system: where's this item coming from? Where's it going? What's it made of? In the case of trunk, it may not be the most refined at the moment, but it's coming from outside the canvas, vertically below, "where modals live". This is acknowledging that on mobile, similar modals behave like a bottom card. If it simply comes from the center and scales up, it doesn't come from anywhere. |
I completely agree with your point about the importance of orchestration, Joen, especially in mobile interfaces. On smaller screens, having the modal originate from the bottom can create a natural, grounded flow that feels connected to the user’s touch and the overall layout. It makes sense for elements to come from where the user’s interactions typically occur, providing a clear and intuitive experience. However, in the context of a desktop interface, I believe that keeping the modal centered has its own distinct advantages. When the modal originates from the center, it instantly captures the user’s attention. The motion starting from where the modal will ultimately rest creates a direct connection between the beginning and end of the animation, making the transition feel more cohesive and less distracting. This seamless movement helps maintain the user's focus on the modal’s content rather than on the motion itself, ensuring that the interaction remains straightforward and unobtrusive. Animations that start from the center and scale up or fade in provide a sense of stability, particularly on desktop screens where the user's eye is naturally drawn to the center. This approach minimizes the amount of eye movement required to track the animation, reducing potential distraction or disorientation that could occur if the modal originated from the bottom and moved upwards. On larger screens, this can be especially important as it allows for a more comfortable and intuitive viewing experience. In terms of storytelling and visual narrative, a modal that grows or fades into place from the center feels like it’s an organic part of the interface. This approach aligns with key motion design principles that emphasize continuity and coherence, suggesting that the modal is an inherent part of the current context rather than an external element entering from an arbitrary location. This integration helps maintain a consistent visual narrative and enhances the overall user experience. I'll explore different modal animations in various context, including both centered and vertical appearances. This should help us compare and find the best solution. These are just my two cents. No matter which option we choose, our main goal should be to help users navigate smoothly and clearly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The animation change looks great 👍
The blur...I'd personally prefer to not have it, because it will still cause performance issues and dropped frames on some browsers, especially when there's a lot to blur in the background.
Also, in some contexts, it might actually make sense if the background is visible, as the user might want to use some of it without closing the dialog (which is fine regardless if it's inert)
My recommendation would be to address blur separately (in another PR), and treat it carefully with some performance measurements on all major browsers. After all, we did remove it once, specifically for performance reasons.
I don't mind removing the blur and discussing that separately. But I would suggest; if users need to see content behind a modal, then it is most probably the incorrect UX. Good discussion about the point of origin!
This resonates with me. While I understand this is a subjective view, my feeling is the modal conceptually emerges directly from the clicked element, rather than existing off-screen, waiting to be triggered. More specifically, it's the contents of the modal that originate from the element – the modal container itself is merely a vessel, and its off-screen position before activation feels largely inconsequential. Given the lack of a practical way to visually represent this idea, and considering that some modals might appear on-load, having them originate from the center of the screen on desktop, with a scale/fade animation seems like a solid alternative to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even without the blur, I think this PR is safe to ship with a changelog added. I'd be happy to get additional feedback from the component team.
7d0f7010558ce5b458008fbdeae3ecfa.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong opinion from the UX standpoint, but code-wise I have no issues 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great, thank you @jameskoster!
I made some adjustments to the animation as suggested by @nick-a8c, and updated the gif in the OP. What do you think @jasmussen? |
Let's merge this one and gather any wider feedback. |
Unlinked contributors: nick-a8c. Co-authored-by: jameskoster <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: mirka <[email protected]> Co-authored-by: hanneslsm <[email protected]> Co-authored-by: paaljoachim <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: jasmussen <[email protected]>
Before we move on, can we look at having a subtle exit animation as well? |
@@ -1,5 +1,5 @@ | |||
@mixin edit-post__fade-in-animation($speed: 0.2s, $delay: 0s) { | |||
animation: edit-post__fade-in-animation $speed ease-out $delay; | |||
@mixin edit-post__fade-in-animation($speed: 0.08s, $delay: 0s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this mixin is defined has a few issues that we should solve:
- the mixin name suggests that the mixin is defined in the
@wordpress/edit-post
package, while it's actually defined in the@wordpress/base-styles
package; - the
edit-post__fade-in-animation
animation keyframes are defined (twice) in separate packages: here and here. That is a potential issue in itself, since:- these two animation definitions can get out of sync (ie. not have the same underlying keyframe specs)
- if a third-party consumer of
@wordpress/base-styles
doesn't need to load the@wordpress/edit-site
or@wordpress/edit-post
packages, then the animation will not work (since it's not defined in@wordpress/base-stlyles
)
The result is that, currently, the Modal
component in @wordpress/components
is consuming a SCSS mixin from @wordpress/base-styles
(which is confusingly prefixed with the name of another package), but it needs for either the @wordpress/edit-site
or the @wordpress/edit-post
packages to be loaded too. This creates an implicit circular dependency that we should break.
Looking at WPDirectory, it looks like the edit-post__fade-in-animation
mixin is used by third-party developers, and therefore we can't really rename it.
But we can still move the @keyframes
definitions in the same file as the mixin, and delete the duplicate definitions in other packages — this will remove implicit circular dependencies, and prevent unexpected behaviors caused by the keyframes potentially having different definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying out the approach suggested above as part of #65203 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted to #65377
Trying it out in #65203 |
@nick-a8c would you mind sharing your WordPress.org username so we can ensure you're properly credited in the 6.7 release next week? |
@jeffpaul |
Currently the
modal
scrim fades in while the dialog moves upwards. I appreciate this is subjective, but I find these animations clash a bit. It's made worse by the fact they occur at slightly different speeds as well.As a mostly for-fun experiment, this PR attempts to polish the animation by tweaking a few details:
scale
andopacity
are utilised so that the modal appears from the center of the screen instead of moving upwards along the Y axis.Before
After
Testing Instructions
What do y'all think?